Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning message for matching on redundant nulls #21850

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

HarrisL2
Copy link
Contributor

Fixes #21724

@HarrisL2 HarrisL2 marked this pull request as draft October 28, 2024 21:48
@HarrisL2
Copy link
Contributor Author

I changed the checkfile for tests/patmat/null.check as the result did seem to be wrong to me.

It seems that there has been some history on this issue, with a commit which specifically does the opposite of this PR, removing null computations because of expensive computation costs.

Would this fix be unnecessary because it incurs a large performance cost?

@HarrisL2 HarrisL2 marked this pull request as ready for review October 28, 2024 23:48
@noti0na1
Copy link
Member

noti0na1 commented Nov 4, 2024

Maybe I need to clarify my suggestion. I mean we always project a wildcard pattern to nullable space if the type is nullable.

For example: case a => and case _ => both will match the null value in runtime. This change should also fix some bug in the existing test suit, and make the redundant check more cleaner.

Like in tests/warn/i20122.scala:

sealed trait T_B[C, D]

case class CC_A()
case class CC_B[A, C](a: A) extends T_B[C, CC_A]
case class CC_C[C, D](a: T_B[C, D]) extends T_B[Int, CC_A]
case class CC_E(a: CC_C[Char, Byte])

val v_a: T_B[Int, CC_A] = CC_B(CC_E(CC_C(null)))
v_a match
  case CC_B(CC_E(CC_C(_))) => println(0) // was warn: unreachable
  case _                   => println(1)

If we run the code, we can see the first case is executed, so it is not unreachable.

Then for the OnlyNull warning, I propose the following scheme. Issue OnlyNull warning only if:

  • The target space is nullable;
  • OnlyNull warning has not been issued before;
  • The pattern is a wildcard pattern;
  • The pattern is not covered by the previous cases, but covered by the previous cases with null.

I tried this approach, I think the code should be more cleaner: https://github.com/dotty-staging/dotty/blob/381c197ebbc1feb2f801c9df31d06e6d4d97fa6a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala

Although this may contradict to what I said in tests/explicit-nulls/warn/i21577.scala...

def f3(s: String | Null) = s match
  case s2: String =>
  case _ => // warn NullOnly or not?

@noti0na1
Copy link
Member

noti0na1 commented Nov 4, 2024

This list of tests should be corrected:

tests/warn/i20122.scala failed
tests/warn/i20121.scala failed
tests/warn/i20123.scala failed

Related to #21000

cc @dwijnand

@noti0na1
Copy link
Member

noti0na1 commented Nov 4, 2024

Maybe you need a rebase?

@HarrisL2
Copy link
Contributor Author

HarrisL2 commented Nov 4, 2024

[warn] -- [E121] Pattern Match Warning: /home/runner/work/scala3/scala3/compiler/src/dotty/tools/dotc/core/SymbolLoaders.scala:340:11 
[warn] 340 |      case _ => NoAbstractFile
[warn]     |           ^
[warn]     |Unreachable case except for null (if this is intentional, consider writing case null => instead).

Looks like there's code in the other tests that use a wildcard to catch a null. Somehow that didn't throw a warning before?

Copy link
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except the weird diff, otherwise LGTM. Can you try to git rebase scala3/main to see if you can get rid of the issue?

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks.

@dwijnand dwijnand merged commit fa43ab8 into scala:main Nov 5, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate warning message for the last redundant wildcard case in pattern match
3 participants